Conversation
Pull reviewers statsStats of the last 120 days for UWOrbital:
|
proprogrammer504
left a comment
proprogrammer504
left a comment
There was a problem hiding this comment.
looks good to me. please correct me if i am wrong, but i am under the assumption that this implementation functions by intercepting backend exceptions during API calls and sends a json back to the client detailing the exception, instead of crashing the backend. but only IF the exception came from a client calling some endpoint?
continuing this, would there be a way to also handle backend exceptions which are not related to endpoints? i believe that this currently cannot handle backend exceptions which are unrelated to endpoints (ie if there was a general process which runs regardless of endpoint calls). |
camspec
left a comment
camspec
left a comment
There was a problem hiding this comment.
Good work overall, but some aspects need to be looked at
| """ | ||
| return JSONResponse( | ||
| status_code=HTTP_400_BAD_REQUEST, | ||
| content={"detail": exc.message}, |
There was a problem hiding this comment.
Any reason why this key is detail and not message?
|
|
||
| def setup_exception_handlers(app: FastAPI) -> None: | ||
| """ | ||
| @brief Add exception handlers to the FastAPI app |
There was a problem hiding this comment.
Make sure you're following our docstring convention for Python
| @attribute exc (BaseOrbitalError) - The exception that was raised | ||
| """ | ||
| return JSONResponse( | ||
| status_code=HTTP_400_BAD_REQUEST, |
There was a problem hiding this comment.
You're returning 400 as a fallback for a lot of exceptions, but this is a client-side error that we're returning, even if it wasn't the client's fault. We should also be more specific.
| @attribute app (FastAPI) - The FastAPI app to add the exception handlers to | ||
| """ | ||
|
|
||
| @app.exception_handler(BaseOrbitalError) |
There was a problem hiding this comment.
When would this base exception handler ever be called? What handler is used in the case of a general exception (fallback)?
3 participants
Purpose
Closes #371 .
Explain the purpose of the PR here if it doesn't match the linked issue. Be sure to add a comment in the linked issue explaining the changes.
New Changes
Added handlers for the custom and common exceptions, and wrote unit tests
Testing
Explain tests that you ran to verify code functionality.
Outstanding Changes
If there are non-critical changes (i.e. additional features) that can be made to this feature in the future, indicate them here.